Skip to content

docs: fix ReferenceError in open-redirect example (Url -> URL) - #2465

Merged
bjohansebas merged 1 commit into
expressjs:mainfrom
ofri-peretz:docs/fix-open-redirect-example-url
Aug 1, 2026
Merged

docs: fix ReferenceError in open-redirect example (Url -> URL)#2465
bjohansebas merged 1 commit into
expressjs:mainfrom
ofri-peretz:docs/fix-open-redirect-example-url

Conversation

@ofri-peretz

Copy link
Copy Markdown
Contributor

Summary

The open-redirect validation example on Production Best Practices: Security calls new Url(req.query.url), but the JavaScript global is URL. As written the constructor throws ReferenceError: Url is not defined on every request, the catch swallows it, and the handler responds 400 "Unsupported redirect" for every URL — including valid ones — so the redirect never runs. Anyone copying the snippet gets a handler that silently rejects all redirects instead of validating them.

One-line fix: UrlURL.

The same snippet appears in the translated mirrors, but per docs/i18n.md I've only touched the English source so Crowdin can propagate it.

Verified locally: npm run check green, unit tests 52/52, and a full astro build succeeds with the corrected snippet in the built page.

The open-redirect validation example calls `new Url(...)`, but the
JavaScript global is `URL`. As written, the constructor throws
ReferenceError on every request, the catch block swallows it, and the
example responds 400 "Invalid url" for every URL - including valid
ones - so the redirect never runs. Anyone copying the snippet gets a
handler that silently rejects all redirects instead of validating them.
@ofri-peretz
ofri-peretz requested a review from a team as a code owner July 31, 2026 21:48
@netlify

netlify Bot commented Jul 31, 2026

Copy link
Copy Markdown

Deploy Preview for expressjscom-preview ready!

Name Link
🔨 Latest commit a466664
🔍 Latest deploy log https://app.netlify.com/projects/expressjscom-preview/deploys/6a6d1844b3815200086e51b5
😎 Deploy Preview https://deploy-preview-2465--expressjscom-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 97 (no change from production)
Accessibility: 100 (no change from production)
Best Practices: 100 (no change from production)
SEO: 100 (no change from production)
PWA: 80 (no change from production)
View the detailed breakdown and full score reports
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@krzysdz krzysdz added bug docs Issues/pr concerning content labels Jul 31, 2026

@krzysdz krzysdz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops. @marco-ippolito found this in #1474 (comment), the suggestion was applied in 401c6a6 and then before merging there was a force push, which reformatted the code and accidentally reverted this change.

@bjohansebas
bjohansebas merged commit 1ca3bfb into expressjs:main Aug 1, 2026
16 of 17 checks passed
@ofri-peretz

Copy link
Copy Markdown
Contributor Author

Thanks @krzysdz for digging up the history, and @bjohansebas for merging.

Worth recording that the credit for spotting it belongs to @marco-ippolito in #1474 — the fix was applied there and then lost to a force-push before merge. I only noticed the symptom. Glad it's back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug docs Issues/pr concerning content

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants